Repository navigation
install: stop re-saving an unchanged lockfile for wildcard deps on versionless workspace members - #37249
install: stop re-saving an unchanged lockfile for wildcard deps on versionless workspace members#37249robobun wants to merge 9 commits into
Conversation
…rsionless workspace members Loading bun.lock rewrites a dependency whose resolution is a workspace to a workspace-tagged dependency, but parsing package.json kept a wildcard range on a versionless workspace member as an npm dependency (parse only mirrored the resolver's rule for members with a version field). The two dependency lists never compared equal, so every install counted one update and re-saved a byte-identical lockfile. Apply the resolver's versionless-member rule at parse time: a wildcard range on a versionless workspace member parses as a workspace dependency, the same as ranges that satisfy a versioned member.
|
Status: ready for review. Reproduced the re-save loop with the released bun (1.4.0-canary.1) and root-caused it to the loaded lockfile rewriting wildcard deps on versionless workspace members into a shape a fresh package.json parse never produces. The fix gates the loader rewrite on the parser's rule while linkWorkspacePackages is on, and keeps the unconditional rewrite when it is off so flipping the setting still re-resolves (head a368e56, same diff as f20bf81). CI: every lane green except darwin 14 x64, where test/napi/napi.test.ts fails identically on main (pre-existing, reported separately); the remaining noise was retry-recovered flakes on lanes this diff does not touch. Verification details in the PR body. |
WalkthroughThe lockfile parser now propagates workspace versions and the workspace-linking option. Dependency rewriting occurs only when linking is enabled and the workspace version satisfies the declared range. A concurrent regression test covers repeated installs with a versionless workspace member. ChangesWorkspace dependency linking
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:44 AM PT - Aug 9th, 2026
❌ @robobun, your commit a368e56 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37249That installs a local version of the PR into your bun-37249 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ting parse The parse-side rewrite regressed package-lock.json migration: a migrated lockfile keeps npm-tagged dependencies, so rewriting the fresh parse to workspace-tagged made the differ discard every affected workspace member and re-resolve its dependencies, losing npm's pinned versions. Fix the mismatch from the loader side instead. bun.lock loading rewrote any dependency whose resolution is a workspace package to a workspace-tagged dependency, including shapes the package.json parser never produces (a wildcard range on a versionless member). Gate that rewrite on the parser's rule: an npm range is rewritten only for a member version the range satisfies. Deps the parser keeps npm-tagged now load npm-tagged, so the loaded and re-parsed root compare equal and the lockfile stops re-saving.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-workspaces.test.ts`:
- Around line 225-253: Update both bun install spawn calls in the test to use
stdout: "ignore" since stdout is not asserted. Await stderr.text() and exited
concurrently for each subprocess, preserving the existing stderr and exit-status
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9003fb0f-da4c-4360-9e1c-a6717e66b764
⛔ Files ignored due to path filters (1)
test/cli/install/__snapshots__/bun-workspaces.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/install/lockfile/bun.lock.rstest/cli/install/bun-workspaces.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/install/lockfile/Package.rs:1755-1763— This change regressestest/cli/install/migration/complex-workspace.test.tson every CI platform (error: File node_modules/iconv-lite was found). The fixture is exactly the shape this PR modifies —packages/lol-package/package.jsonis{"name":"lol"}with no version field andpackages/second/package.jsondepends on it via"lol": "*"— so the newNone => workspace_path.is_some() && ...is_star()arm now rewrites that dep to workspace-tagged during the migration parse where it previously stayed npm-tagged, changing the resulting hoist. Either the new layout is correct and the fixture'smustNotExistexpectations need updating, or the parse-time rewrite needs scoping so npm-lockfile migration is unaffected.Extended reasoning...
What is failing
CI reports
test/cli/install/migration/complex-workspace.test.tsfailing with the identical assertion on 8+ platforms (linux x64/aarch64/asan, alpine, ubuntu, windows x64/aarch64):error: File node_modules/iconv-lite was foundwhich is
mustNotExist("node_modules/iconv-lite")at complex-workspace.test.ts:121. This is not a network flake — the assertion is that a file exists which previously did not, meaning install succeeded but produced a differentnode_moduleslayout. An identical deterministic failure on every platform is a behavior change.Why this PR is the cause
The fixture contains precisely the input shape this PR changes:
test/cli/install/migration/complex-workspace/packages/lol-package/package.jsonis{ "name": "lol", "dependencies": {...} }— noversionfield.test/cli/install/migration/complex-workspace/packages/second/package.jsonhas"lol": "*"independencies.
Before this PR,
parse_dependency'sTag::Npmarm was entirely wrapped inif let Some(workspace_version) = workspace_version { ... }. Withlolhaving no version,workspace_versionisNone, so the block was skipped andsecond's"lol": "*"stayed npm-tagged during the migration parse.After this PR (Package.rs:1759-1763), the
Nonearm computessatisfies = workspace_path.is_some() && dependency_version.npm().version.is_star().lolis inworkspace_pathsand"*"is star, sosatisfiesis nowtrueand the dependency is rewritten to workspace-tagged viaparse_with_tag(..., Tag::Workspace, ...).Step-by-step
complex-workspace.test.tscopies the fixture, then runsbun installwhich migrates from the checked-in npmpackage-lock.json.- Migration invokes
Diff::generate(Package.rs:1420), which callsworkspace_pkg.parse_with_json(to_lockfile, ...)for each workspace member — includingsecond. to_lockfile.workspace_pathsis populated from the root parse and containslol, so whenparse_dependencyhandlessecond's"lol": "*",workspace_path.is_some()is true.- Old behavior:
workspace_versionisNone→ whole block skipped → dep staysTag::Npm.
New behavior:Nonearm →is_star()→satisfies = true→ dep becomesTag::Workspace. - The re-tagged dependency changes what the migration diff/hoist computes for
second's subtree (which pulls inbody-parser→iconv-litetransitively via the fixture's aliased deps), andiconv-litenow lands at rootnode_modules/where the test asserts it must not.
The PR description's local-green list (
bun-workspaces,bun-lock,migrate-bun-lockb-v2, etc.) does not includecomplex-workspace.test.ts, so this suite was not run before pushing.Why nothing prevents it
The PR intentionally removes the
workspace_version.is_some()gate on the workspace-rewrite path — that is the fix. Butparse_dependencyruns on both the steady-state re-parse path (where matchingbun.lock.rs'smap_dep_to_pkgshape is desired) and the npmpackage-lock.jsonmigration path viaDiff::generate. The migration fixture's expectations were written against the old npm-tagged parse.Impact
Per REVIEW.md: "Treat every refactor as guilty until proven behavior-preserving" and "When changing output/defaults/messages, grep the suite for assertions on the old behavior and update them in the same PR." An all-platform deterministic failure of a previously-passing test whose fixture contains the exact modified input is a regression that must be resolved before merge.
How to fix
Run
bun bd test test/cli/install/migration/complex-workspace.test.tslocally and determine which side is correct:- If the new hoisting is the intended outcome of linking
lolas a workspace during migration, update the fixture'smustNotExist("node_modules/iconv-lite")(and any adjacent layout assertions that also moved) to match, and note the intentional layout change in the PR description. - If npm-lockfile migration should not be affected, scope the
None => is_star()rewrite so it does not fire on the migration parse path (or otherwise preserve the previous dep shape there).
|
Right, and that CI failure is what prompted the redesign: the parse-side rewrite was reverted in 9aded96 (the finding describes the diff before that). The current head fixes the re-save from the loader side instead (map_dep_to_pkg only rewrites a dependency into the shape the parser produces), so the migration parse path is untouched and complex-workspace's fixture shape (versionless |
There was a problem hiding this comment.
I reviewed the redesigned fix (loader-side gate in map_dep_to_pkg) and didn't find any bugs. Because it changes how the lockfile loader tags workspace-resolved dependencies — with 5 existing snapshot updates and subtle interaction with the linkWorkspacePackages option — a human look at the loader/parser/resolver invariant would still be worthwhile.
What was reviewed:
- Verified the loader's new predicate (
link_workspace_packages && workspace_versions.get(hash).is_some_and(|v| range.satisfies(v))) mirrorsPackage::parse_dependency's Npm arm exactly, including thenpm.namealias key. - Confirmed
lockfile.workspace_versionsis populated (line ~2059) before the threemap_dep_to_pkgcall sites borrow it, andstring_bufis the same buffer both sides slice from. - Checked the early return still runs
resolutions[dep_id] = pkg_idfirst, so the dep stays mapped to the workspace package; snapshotpackage_idvalues are unchanged. - The new test drains stderr/exit concurrently, ignores stdout, and asserts byte-identical
bun.lockon the second install.
Extended reasoning...
Overview
The PR fixes a lockfile re-save loop for wildcard (*) dependencies on versionless workspace members. The final approach gates the loader's npm→workspace dependency-tag rewrite in map_dep_to_pkg (src/install/lockfile/bun.lock.rs) on the same predicate Package::parse_dependency uses, so a loaded lockfile's dependency shapes match a fresh package.json parse and the differ stops flagging a spurious update. It threads link_workspace_packages, workspace_versions, and string_buf through three call sites, adds a regression test in bun-workspaces.test.ts, and updates 5 existing snapshots where the loaded dep now keeps its npm tag while package_id still points at the workspace package.
Security risks
None. This is package-manager lockfile diffing/loading logic; no untrusted-input parsing surface, auth, crypto, or path handling is added.
Level of scrutiny
Moderate-to-high. The diff itself is small (~30 lines of new logic), but it sits at the intersection of three subsystems that must agree on dependency shape: the parser (parse_dependency), the resolver (get_or_put_resolved_package), and the loader (map_dep_to_pkg). The PR already went through one redesign after CI's migration/complex-workspace test caught the earlier parse-side approach breaking npm-pinned resolutions on package-lock.json migration. My own prior review on the earlier revision found a pre-release-version divergence. The current loader-side approach is cleaner and I traced the predicate against Package.rs:1752-1792 — it matches. But the 5 snapshot updates change the in-memory tag representation for existing test cases, and the link_workspace_packages gate introduces a new dependency on runtime config inside the loader; the toggle-between-installs scenario (lockfile written with linking on, loaded with linking off) is worth a maintainer sanity check even though the author reports hand-testing it.
Other factors
All review threads are resolved (comment-cop, CodeRabbit's stdout-drain nit, my earlier finding). The regression test is well-constructed: it covers both root and member wildcard deps, asserts "Saved lockfile" appears on the first install and not the second, checks the workspace resolution in bun.lock, and asserts byte-identical lockfile content. The author ran the workspace/lockfile/migration/add/update suites locally. Existing toMatchNodeModulesAt assertions still pass per the PR body, which is the strongest signal that the snapshot changes are representation-only.
With linkWorkspacePackages turned off after a lockfile linked a member, keeping the parsed npm tag made the loaded root match the fresh parse, so the stale workspace resolution was silently kept forever. Rewriting in that state restores the pre-change behavior: the diff fires and the dependency re-resolves under the current setting.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches the lockfile loader's dependency-shape invariant (which must stay in sync with Package::parse_dependency and the resolver) and changes the in-memory representation snapshotted by an existing test, a human look would still be worthwhile.
What was reviewed:
- Confirmed
parse_would_rewriteexactly mirrors the parser's Npm-arm rewrite condition (Package.rs:1752-1792):link_workspace_packages && workspace_version.is_some() && satisfies, keyed onnpm.namevia the samestring_hash. - Confirmed
lockfile.workspace_versionsis populated (bun.lock.rs:2059) before it's borrowed formap_dep_to_pkg(bun.lock.rs:2781), andstring_bufisbuffers.string_bytes. - Snapshot updates keep
package_id: 2(still resolves to the workspace member) — only the loaded dependency's tag changes;toMatchNodeModulesAtstill passes per the evidence block. - The
linkWorkspacePackages=falsepath keeps the unconditional rewrite so flipping the setting still diffs — behavior unchanged from main.
Extended reasoning...
Overview
This PR fixes a re-save loop where bun install rewrites an unchanged bun.lock on every run when a wildcard (*) dependency targets a versionless workspace member. It touches src/install/lockfile/bun.lock.rs (31 lines: thread link_workspace_packages + workspace_versions into map_dep_to_pkg, add a conditional early-return), adds a regression test in bun-workspaces.test.ts, and updates 5 entries in the corresponding snapshot file. An earlier revision that modified Package::parse_dependency was reverted after CI caught it breaking package-lock.json migration; the current loader-side fix leaves parse, migration, and resolution untouched.
Security risks
None. No untrusted input parsing changes, no auth/crypto/permissions, no network paths. The change only adjusts when the loader rewrites an already-parsed dependency's tag from Npm to Workspace.
Level of scrutiny
Moderate-to-high. The install lockfile diff path is subtle: three code sites (parser, loader, resolver) must agree on dependency shape or every install spuriously re-saves or, worse, silently keeps stale resolutions. This PR went through one redesign after CI caught a migration break, and it changes what parseLockfile produces for existing lockfiles (visible in the snapshot diff). I verified the new predicate is byte-for-byte the parser's rewrite condition — same hash function on the same npm.name field, same satisfies call, same link_workspace_packages gate — and that the resolution slot is still written before the early return. But a maintainer who knows this subsystem's history should confirm the snapshot representation change (npm-tagged dependency with workspace package_id) has no downstream consumers that key on the tag.
Other factors
My earlier inline comment on this PR flagged a pre-release divergence in the since-reverted Package.rs change; the author addressed it by moving the fix to the loader and explicitly scoping the pre-release flavor to #37248 (pre-existing on main). All comment-cop and CodeRabbit feedback is resolved. The evidence block shows the new test fails on main (both ASAN and release) and passes with the fix. The is_none_or default of true for link_workspace_packages matches PackageManagerOptions's default.
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The loader gate now mirrors Package::parse_dependency's rule exactly and the resolution assignment happens before the early return, so the fix looks correct. Since this touches the lockfile loader/diff path and already went through one redesign after CI caught a migration regression, a maintainer look at the install/lockfile invariants would still be worthwhile.
What was reviewed:
- Verified the new gate at
map_dep_to_pkgmatches the parser's Npm-arm condition (Package.rs:1752-1792):link_workspace_packages && workspace_version.is_some() && satisfies, keyed onnpm.name(alias-correct). - Confirmed
resolutions[dep_id] = pkg_idruns before the early return, so the dep still resolves to the workspace member; snapshot updates showpackage_idunchanged with the npm tag preserved. - Checked
workspace_versionsis populated (line ~2059) before all threemap_dep_to_pkgcall sites, andstring_bufisbuffers.string_bytes. - The linking-disabled path deliberately keeps the unconditional rewrite;
is_none_ordefaults totruematching the option default.
Extended reasoning...
Overview
This PR fixes a lockfile re-save loop: when a wildcard (*) dependency targets a versionless workspace member, every bun install rewrote a byte-identical bun.lock. The root cause is a shape mismatch between the lockfile loader (which unconditionally rewrote workspace-resolved deps to workspace-tagged) and the package.json parser (which keeps such a dep npm-tagged when the member has no version to satisfy). The fix threads link_workspace_packages, workspace_versions, and string_buf into map_dep_to_pkg and gates the rewrite on the parser's exact rule. Files touched: src/install/lockfile/bun.lock.rs (+31), a new regression test in bun-workspaces.test.ts, and 5 snapshot updates reflecting the in-memory tag change.
Security risks
None. This is internal lockfile-loading logic with no untrusted-input parsing changes, no network/filesystem behavior changes, and no auth/crypto surface.
Level of scrutiny
High. The lockfile loader/parser/resolver agreement is a subtle invariant — a mismatch here silently re-saves lockfiles or, as the earlier revision showed, drops migrated npm pins. This PR already went through one redesign after CI caught migration/complex-workspace breaking on the parse-side approach. The current loader-side fix is narrower and I verified it mirrors parse_dependency line-for-line, but a maintainer who owns the install diff path should confirm the linking-disabled branch (unconditional rewrite to force re-resolve) is the right call versus the alternative of matching parser output there too.
Other factors
- My earlier inline finding (pre-release wildcard divergence in the parse-side approach) was addressed by reverting to the loader-side design; the pre-release flavor is explicitly out of scope and handed to #37248 which restructures that arm.
- All bot feedback (comment-cop, CodeRabbit stdout drain) is resolved.
- Evidence gate passed: 2 tests fail on main (ASAN + release), all 64 pass with the fix; the new test's failure mode on main is the exact snapshot diff and "Saved lockfile" assertion this PR targets.
- The snapshot updates are correct: only the dependency
tagchanges (workspace → npm),package_idand the on-diskbun.lockbytes are unchanged, andtoMatchNodeModulesAtstill passes.
|
Heads up: #37264 widens the parse predicate in |
|
Overlap note: #38869 covers this shape (a range on a versionless member stays npm-tagged on load) as one case of a general rule in the loader, and also handles the same name listed in two dependency groups, which the |
|
Closing: #41302 fixed this on main. It uses one workspace-link rule in the parser, the resolver, and the bun.lock loader. I checked with a debug build of main (4661e49):
|
Repro
Every install rewrites
bun.lockforever, while--frozen-lockfilepasses and the content never changes. Same loop when the wildcard dependency lives in another workspace member, including after a package-lock.json migration.Cause
A
*range on a versionless workspace member is linkable only through the resolver's rule from #10899 (get_or_put_resolved_package: a versionless member satisfies a wildcard).Package::parse_dependencykeeps such a dependency npm-tagged (it only rewrites to a workspace dependency for a member version the range satisfies), but loadingbun.lockrewrote every dependency whose resolution is a workspace package to a workspace-tagged dependency (map_dep_to_pkg). The loaded and freshly parsed roots then never compare equal:Version::eqlfails on the tag mismatch, the diff counts one update,had_any_diffsstays true, and the install re-saves an unchanged lockfile. Members with a version field don't loop because parse rewrites their satisfied ranges to the same shape the loader produces.Fix
Gate the loader's rewrite on the parser's rule: while
linkWorkspacePackagesis on, an npm-tagged dependency is rewritten to a workspace dependency only when the member has a version the range satisfies, i.e. exactly when re-parsing package.json would produce a workspace dependency for it. Dependencies the parser keeps npm-tagged (a wildcard on a versionless member) now load npm-tagged too, the dependency lists compare equal, and the re-save stops. The lookup uses the dependency's real npm name, matching the parser's alias handling, and non-npm tags (workspace:ranges and the injected member entries) keep the existing normalization.With
linkWorkspacePackagesoff the rewrite stays unconditional: a lockfile that previously linked a member must keep diffing against the fresh parse so flipping the setting re-resolves instead of silently keeping the stale link. Today that re-resolution surfaces the pre-existing name-collision error (DependencyLoop) for this shape, byte-identical to an unmodified build (#37248 is fixing that collision).Resolution is unchanged: the dependency still maps to the workspace package on load, which is the same in-memory state a fresh install produces (the resolver links the wildcard to the member without retagging the dependency). The saved lockfile bytes are identical before and after, existing lockfiles load clean and stop being rewritten, and
lockfileVersion: 0lockfiles take one migration save and then stabilize.An earlier revision made the parser rewrite the dependency instead; CI caught it breaking package-lock.json migration (
migration/complex-workspace): migrated lockfiles keep npm-tagged dependencies, so the parse-side rewrite made the differ discard affected members and re-resolve their dependencies, dropping npm's pins. Fixing the loader leaves parse, migration, and resolution untouched.Known adjacent flavors, deliberately out of scope: a member with a pre-release version depended on via
*still re-saves on every install (on an unmodified build too; the resolver's wildcard rule links it while the parser's override branch displaces it, the exact arm #37248 restructures), and the dist-tag fallback flavor sits in #35468's area. Neither overlaps this diff.Verification
New test in
test/cli/install/bun-workspaces.test.ts: root and member wildcard dependencies on a versionless member, second install must not print "Saved lockfile" andbun.lockmust be byte-identical, with the dependency still resolving tono-deps@workspace:packages/no-deps. Fails on an unmodified build (second install prints "Saved lockfile"), passes with the fix. Snapshot updates in the same file show the only in-memory change: the loaded wildcard dependency keeps its npm tag while still resolving to the member (package_idunchanged), andtoMatchNodeModulesAtstill passes.Locally green:
bun-workspaces(64),isolated-install(62),bun-lock(17),lockfile-only,lockfile-version-2,bun-lockb,migrate-bun-lockb-v2,migration/migrate(21),bad-workspace,bun-add(54),bun-update(6). Also checked by hand: hoisted and isolated linkers converge with a stable lockfile, a lockfile written by an older bun is accepted without a rewrite,--frozen-lockfilepasses, a package-lock.json workspace migration produces the same lockfile as an unmodified build and then stays stable, and flippinglinkWorkspacePackagesoff behaves identically to an unmodified build.migration/complex-workspacefails locally only on gitlab/bitbucket network access, identically without this diff.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file